Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add keyword arguments to spectrogram #657

Merged
merged 7 commits into from
Sep 14, 2024
Merged

Add keyword arguments to spectrogram #657

merged 7 commits into from
Sep 14, 2024

Conversation

v923z
Copy link
Owner

@v923z v923z commented Jan 28, 2024

Details and rationale are outlined in https://github.com/v923z/micropython-ulab/blob/spectrum/docs/ulab-utils.ipynb, but the gist is that spectrogram now allows for the re-use of allocated memory (through the out, and scratchpad keyword arguments), as well as direct calculation of the logarithm (through the log keyword argument).

from ulab import numpy as np
from ulab import utils as utils

n = 1024
t = np.linspace(0, 2 * np.pi, num=1024)

scratchpad = np.zeros(2 * n) # re-usable RAM for the calculation of the FFT

for _ in range(10):
    signal = np.sin(t)  # this simulates a measurement of some signal
    utils.spectrogram(signal, out=signal, scratchpad=scratchpad, log=True)

@jepler, @dhalbert this might be relevant for circuitpython users.

@brainstorm
Copy link

brainstorm commented Jul 23, 2024 •
edited
Loading

this might be relevant for circuitpython users.

And for micropython ones too! I was looking for that log component (because of an audio application I'm working on). Plus that RAM-saving scratchpad feature is a good addition too.

Please get this merged! ;)

@v923z
Copy link
Owner Author

v923z commented Jul 24, 2024

Please get this merged! ;)

If you have any comments on the concept itself, please, let me know! This is the best I could come up with, but that doesn't mean that this is the best.

@brainstorm
Copy link

Please get this merged! ;)

If you have any comments on the concept itself, please, let me know! This is the best I could come up with, but that doesn't mean that this is the best.

Sure, fair enough. I just built a runtime with this for esp32s3, tested it and some -inf values returned from my data stream surprised me but they make sense (and they are easily filterable w/ conditionals)... also the kwargs make sense to not break backwards compat. Other than that, LGTM... for sure I'd focus a bit more on improving perf as much as possible before merging since any optimization can save a ton of time at scale.

I'll keep playing with it this and next week and I'll let you know if anything else arises.

@brainstorm
Copy link

There was a minor conflict when merging with master btw, could you please rebase this PR and see if CI complains?

@v923z
Copy link
Owner Author

v923z commented Jul 25, 2024 •
edited
Loading

There was a minor conflict when merging with master btw, could you please rebase this PR and see if CI complains?

The actual conflict is just the version number of ulab, which is trivial. The other one from the CI is mentioned here: #669. Basically, for whatever reason, the executable on linux and on mac doesn't produce the exact same results in a couple of test cases.

There are two possible solutions to this: we either modify the tests in such a way that the difference disappears, or I add the precision keyword argument to set_printoptions https://numpy.org/doc/stable/reference/generated/numpy.set_printoptions.html#numpy-set-printoptions, and simply set the precision to a number, where the rounding errors disappear.

I would prefer the second solution, because it's cleaner, doesn't sweep anything under the rug, and adds a potentially useful feature. I'll just have to find out, how much it costs, although, I believe, it shouldn't be more than 200-300 bytes.

@v923z v923z merged commit c0b3262 into master Sep 14, 2024
25 of 32 checks passed
@v923z v923z deleted the spectrum branch September 14, 2024 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@v923z @brainstorm